Cluster-autoscaler: cloud provider interface implementation for AWS #1377

osxi · 2016-07-15T22:30:54Z

Part of #1311

This change is

googlebot · 2016-07-15T22:30:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

wojtek-t · 2016-07-19T06:53:00Z

@mwielgus @piosz @fgrzadkowski

osxi · 2016-07-19T15:01:19Z

I signed it!

googlebot · 2016-07-19T15:01:21Z

CLAs look good, thanks!

mwielgus · 2016-07-19T19:47:53Z

cc: @andrewsykim @pbitty @iterion

iterion · 2016-07-19T21:36:23Z

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

+}
+
+// MaxSize returns maximum size of the node group.
+func (asg *Asg) MaxSize() int {


It seems we could just pull MaxSize and MinSize from the AutoScalingGroup when we initialize. It's kind of annoying that it must be specified twice, both here and in the ASG itself. I'm not sure if there would be a use case for that?

@mwielgus I think this was lifted pretty much verbatim from the GCE side. I assume (from my short search through the MIG docs) that MIGs don't allow you to specify a min/max?

That makes sense to me.

@iterion No, migs allow you only to specify the target number of instances.

googlebot · 2016-07-26T15:04:16Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

andrewsykim · 2016-07-26T15:10:15Z

I signed it!

mwielgus · 2016-07-26T22:26:53Z

@pbitty @iterion Are you ok with the implementation? Did you have a chance to test it?

iterion · 2016-07-26T23:00:49Z

We haven't tested the most recent iteration, but there were only minor changes. We'll try to test it all again soon.

andrewsykim · 2016-07-26T23:35:36Z

Same thing on our side, hopefully this week we'll get a chance to test it. We're also looking into writing some e2e tests for the AWS implementation.

osxi · 2016-07-27T18:46:44Z

I just tested it out and it works good. Turned up cluster using kops and was able to scale the worker ASG up/down as expected.

mwielgus · 2016-08-04T12:54:10Z

@osxi could you please sign the CLA?

mwielgus · 2016-08-04T12:55:22Z

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go

+// 	assert.NoError(t, err)
+// }
+
+// TODO: IncreaseSize


Please implement.

osxi · 2016-08-04T22:04:26Z

@mwielgus I signed the CLA but then I added a commit from @andrewsykim, who also signed the CLA.

I'll implement the commented tests.

andrewsykim · 2016-08-06T15:51:36Z

Finally had some time today to test this out! Sorry it took so long, we had to upgrade to v1.3 to get it working. The implementation works as expected! The only issue I've run into was that the Node objects are not deleted after scale down and they're set to a NotReady state which leaves a bunch of resources dangling (especially daemonsets). I don't think this issue is within the scope of this PR, I'm assuming its the responsibility of the API server to delete the node object if the instance is terminating. But I can also see how this problem can become an issue for clusters that run the CA for extended periods of time, wondering if the CA should explicitly tell the API server to delete the node that its terminating as a temporary solution to this problem (again, probably outside the scope of this PR).

davidopp · 2016-08-07T22:40:52Z

Node object should be automatically deleted by node controller once it is gone from the cloud provider:

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/node/nodecontroller.go#L469

osxi · 2016-08-08T18:58:54Z

Re-tested after adding the AWS SDK autoscaler interface around the autoscaler service and still works.

I haven't experienced any dangling nodes. After chatting with @andrewsykim on Slack, it looks like that might be related to the use of EC2 Classic (where my testing took place on a VPC setup).

mwielgus · 2016-08-08T19:13:14Z

'golint' problems: 
/home/travis/gopath/src/k8s.io/contrib/cluster-autoscaler/cloudprovider/aws/aws_manager.go:45:6: exported type AutoScaling should have comment or be unexported

osxi · 2016-08-08T21:25:03Z

@mwielgus I will fix that

osxi · 2016-08-15T14:29:27Z

@mwielgus can you please manually approve the CLA?

mwielgus · 2016-08-16T10:36:37Z

There is no approval for CLA. There must be something missing on your side. Please check if your git author settings are in line with your github profile.

googlebot · 2016-08-16T21:09:17Z

CLAs look good, thanks!

mwielgus · 2016-08-16T21:31:52Z

The code looks ok. Please squash the commits to 1-2 commits (code / extra godeps) and we should be ready to merge.

osxi · 2016-08-16T21:38:53Z

@mwielgus done

mwielgus · 2016-08-16T21:42:19Z

LGTM

mwielgus · 2016-08-16T21:42:52Z

Thank you for your contribution! :)

osxi · 2016-08-16T21:56:19Z

You're very welcome!

k8s-github-robot · 2016-08-16T22:00:00Z

Automatic merge from submit-queue

pbitty · 2016-08-16T22:01:42Z

Awesome. Nice work @osxi and @andrewsykim !

mwielgus · 2016-08-18T08:46:35Z

Please remember to update Kubernetes kube_up.sh scripts for AWS as well :).

Automatic merge from submit-queue Bump cluster autoscaler to 0.3.0-beta2 Contains belongs fix kubernetes-retired/contrib#1560 and AWS support kubernetes-retired/contrib#1377.

…/topic/aws-provider Automatic merge from submit-queue Cluster-autoscaler: cloud provider interface implementation for AWS Part of kubernetes-retired/contrib#1311  --- This change is [<img src="https://reviewable.kubernetes.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.kubernetes.io/reviews/kubernetes/contrib/1377)

googlebot added the cla: no label Jul 15, 2016

piosz assigned mwielgus Jul 19, 2016

googlebot added cla: yes and removed cla: no labels Jul 19, 2016

mwielgus added the area/autoscaler label Jul 19, 2016

iterion reviewed Jul 19, 2016
View reviewed changes

googlebot added cla: no and removed cla: yes labels Jul 26, 2016

mwielgus assigned mwielgus and unassigned mwielgus Jul 26, 2016

mwielgus reviewed Aug 4, 2016
View reviewed changes

googlebot added cla: yes and removed cla: no labels Aug 16, 2016

osxi added 2 commits August 16, 2016 16:37

Cluster-autoscaler: cloud provider interface implementation for AWS

c114e42

add github.com/stretchr/testify/mock to vendor

ddeabd4

mwielgus added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2016

k8s-github-robot merged commit 0264897 into kubernetes-retired:master Aug 16, 2016

mwielgus mentioned this pull request Aug 18, 2016

Bump cluster autoscaler to 0.3.0-beta2 kubernetes/kubernetes#30852

Merged

philk mentioned this pull request Aug 24, 2016

AWS: We should consider updating/reusing cluster-autoscaler to support AWS kubernetes/kubernetes#11935

Closed

andrewsykim mentioned this pull request Nov 17, 2016

cluster autoscaler addon kubernetes/kops#914

Merged

mwielgus mentioned this pull request Oct 1, 2018

Add Alibaba Cloud Provider support kubernetes/autoscaler#1277

Closed

cblecker unassigned mwielgus Apr 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster-autoscaler: cloud provider interface implementation for AWS #1377

Cluster-autoscaler: cloud provider interface implementation for AWS #1377

osxi commented Jul 15, 2016 •

edited by thockin

Loading

googlebot commented Jul 15, 2016

wojtek-t commented Jul 19, 2016

osxi commented Jul 19, 2016

googlebot commented Jul 19, 2016

mwielgus commented Jul 19, 2016

iterion Jul 19, 2016

pbitty Jul 19, 2016

mwielgus Jul 20, 2016

googlebot commented Jul 26, 2016

andrewsykim commented Jul 26, 2016

mwielgus commented Jul 26, 2016

iterion commented Jul 26, 2016

andrewsykim commented Jul 26, 2016 •

edited

Loading

osxi commented Jul 27, 2016 •

edited

Loading

mwielgus commented Aug 4, 2016

mwielgus Aug 4, 2016

osxi commented Aug 4, 2016 •

edited

Loading

andrewsykim commented Aug 6, 2016

davidopp commented Aug 7, 2016

osxi commented Aug 8, 2016

mwielgus commented Aug 8, 2016

osxi commented Aug 8, 2016

osxi commented Aug 15, 2016

mwielgus commented Aug 16, 2016

googlebot commented Aug 16, 2016

mwielgus commented Aug 16, 2016

osxi commented Aug 16, 2016

mwielgus commented Aug 16, 2016

mwielgus commented Aug 16, 2016

osxi commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

pbitty commented Aug 16, 2016

mwielgus commented Aug 18, 2016

Cluster-autoscaler: cloud provider interface implementation for AWS #1377

Cluster-autoscaler: cloud provider interface implementation for AWS #1377

Conversation

osxi commented Jul 15, 2016 • edited by thockin Loading

googlebot commented Jul 15, 2016

wojtek-t commented Jul 19, 2016

osxi commented Jul 19, 2016

googlebot commented Jul 19, 2016

mwielgus commented Jul 19, 2016

iterion Jul 19, 2016

Choose a reason for hiding this comment

pbitty Jul 19, 2016

Choose a reason for hiding this comment

mwielgus Jul 20, 2016

Choose a reason for hiding this comment

googlebot commented Jul 26, 2016

andrewsykim commented Jul 26, 2016

mwielgus commented Jul 26, 2016

iterion commented Jul 26, 2016

andrewsykim commented Jul 26, 2016 • edited Loading

osxi commented Jul 27, 2016 • edited Loading

mwielgus commented Aug 4, 2016

mwielgus Aug 4, 2016

Choose a reason for hiding this comment

osxi commented Aug 4, 2016 • edited Loading

andrewsykim commented Aug 6, 2016

davidopp commented Aug 7, 2016

osxi commented Aug 8, 2016

mwielgus commented Aug 8, 2016

osxi commented Aug 8, 2016

osxi commented Aug 15, 2016

mwielgus commented Aug 16, 2016

googlebot commented Aug 16, 2016

mwielgus commented Aug 16, 2016

osxi commented Aug 16, 2016

mwielgus commented Aug 16, 2016

mwielgus commented Aug 16, 2016

osxi commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

pbitty commented Aug 16, 2016

mwielgus commented Aug 18, 2016

osxi commented Jul 15, 2016 •

edited by thockin

Loading

andrewsykim commented Jul 26, 2016 •

edited

Loading

osxi commented Jul 27, 2016 •

edited

Loading

osxi commented Aug 4, 2016 •

edited

Loading